fix: detect font-scoped garbled text - #365
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes mixed-font pages bypassing garbled-text detection by augmenting analyze_text_quality with per-font CipherGarbleStats accumulation (while retaining the existing page-wide aggregate as a backstop), addressing the dilution issue described in #352.
Changes:
- Track substitution-cipher/garble statistics both page-wide and per-font (keyed by
TextItem.font), skipping empty font names in the per-font map while still counting them in the page-wide aggregate. - Route a page to OCR if either the page-wide stats or any statistically meaningful per-font stats meet the existing “looks garbled” thresholds.
- Add regression tests covering mixed healthy+garbled fonts, clean multi-font pages, and cross-font aggregation behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/text_quality.rs | Adds per-font CipherGarbleStats accumulation and updates OCR-routing to consider both page-wide and per-font evidence. |
| src/lib.rs | Adds targeted unit tests validating mixed-font garble detection and preserving prior aggregation behavior across items/fonts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Shadow auto-approve: would not auto-approve because issues were found.
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would auto-approve. Focused bug fix adds per-font cipher-garble detection to catch mixed-font garbled text; preserves existing thresholds and public API, and adds comprehensive regression tests pinning the corrected behavior.
Re-trigger cubic
|
One consumer/API boundary worth making explicit: my comparison harness evaluates I agree the default fast classifier should not silently become a full extraction. Is the intended contract that callers needing encoding-quality routing use the extraction API, or would maintainers prefer a separate follow-up for an opt-in analyzed-classification API (text-quality routing without Markdown generation)? Either is workable; documenting that boundary would help consumers avoid treating |
|
@knealfortegra Thanks for calling this out. I confirmed the boundary and documented it in For this PR, the current contract remains: I updated the Rust public API docs, Python binding docs and type stubs, both language guides, the README, and the PR description. I deliberately did not make the fast classifier perform extraction. If maintainers want a separate opt-in analyzed-classification API—particularly for Python callers that want text-quality routing without Markdown—that would be a clean follow-up rather than an expansion of this focused fix. The documentation-only follow-up passed formatting, clippy, the full Rust test suite (883 library, 2 binary, 162 integration, 2 doc tests), release build, version check, and the native WASM package check. |
There was a problem hiding this comment.
0 issues found across 6 files (changes from recent commits).
Shadow auto-approve: would require human review. Adds per-font garble heuristics and Base64 aggregate discriminator, changing which pages are flagged for OCR routing; heuristic thresholds and policy are product tradeoffs needing human review. Diff truncation also prevents verifying the full Base64 logic.
Re-trigger cubic
…-garble-stats # Conflicts: # docs/python.md
|
Resolved the The repository CI run is currently waiting for maintainer approval because this is an external-contributor PR: https://github.com/firecrawl/pdf-inspector/actions/runs/32109052130 Could a maintainer approve that run when convenient? |
…-garble-stats # Conflicts: # src/lib.rs
|
Synced this branch with current main in 1cc9204 and resolved the src/lib.rs conflict by keeping both the current supplemental-OCR tests and this PR's font-scoped test helper. The refreshed branch passes root and wasm formatting, default and OCR-feature Clippy with -D warnings, default tests (1041 library, 3 binary, 165 integration, 2 doc), OCR-feature tests (1129 library, 4 binary, 165 integration, 2 local-render, 5 OCR, 2 doc), release build, version-sync check, 18 script tests, and native wasm cargo check. @abimaelmartell, could you please review the updated branch and approve the external-contributor CI run when convenient? The PR is mergeable again. |
Fixes #352.
Root cause
analyze_text_qualityaccumulated a singleCipherGarbleStatssample for the whole page. On mixed-font pages, enough healthy text from one font could dilute shifted or garbled text from another font, so the page-wide histogram passed and corrupt text could be served silently.Fix
TextItem.font; empty font names remain page-wide only.TextItemwithout a trailing space.TextItemboundaries. Exempt only valid Base64 framing with either Base64-specific symbols/padding or an overwhelmingly printable decoded payload.The aggregate structured-data gate replaces the earlier broad Base64 percentage exemption. It preserves digit-heavy shifted tables and word-fragmented PDFs without allowing encoded chunks to become false prose evidence. This does not change the public API, page-wide cipher thresholds, or the runtime semantics of
classify_pdf*/detect_pdf*.API boundary
Fast
classify_pdf*anddetect_pdf*calls remain structural classifiers: they do not extract text or validate font/character encoding, sopages_needing_ocr=[]is not an encoding-quality verdict. Encoding-quality OCR routing usesextract_pages_markdown*or the fullprocess_pdf*APIs; Rust callers can also selectProcessMode::Analyzeto run text-quality analysis without Markdown generation.Commit
4644ac9makes this contract explicit in the Rust API docs, Python binding docs and type stubs, the Python guide, the Rust guide, and the README. An opt-in analyzed-classification API can be considered separately if maintainers want that additional public surface.Regression coverage
FixtureFontsample is flagged assuspected_garbled_text.TextItems.The word-item garble and short-wrapped Base64 regressions both failed before
d3ad416and pass with the aggregate discriminator.I also reran the self-contained ReportLab/pypdf reproduction from #352 against the release
detect-pdf --analyze --jsonpath after merging currentmain(74ebce4), which now reports actual BaseFont family names onTextItem.font:suspected_garbled_text.suspected_garbled_text.Current-main sync
Merge commit
be0267eresolves the documentation conflict with currentmainwhile preserving both sides of the API contract: the newly added selectiveprocess_pdf_with_ocr*APIs are documented, and fastclassify_pdf*/detect_pdf*calls remain explicitly structural-only. The PR remains limited to its intended seven-file delta against currentmain.Verification
cargo fmt --all -- --checkcargo fmt --manifest-path wasm/Cargo.toml -- --checkcargo clippy -- -D warningscargo clippy --features ocr -- -D warningscargo test --quiet(965 library, 3 binary, 162 integration, and 2 doc tests passed)cargo test --features ocr(1046 library, 4 binary, 162 integration, 2 local-render, 5 OCR, and 2 doc tests passed)cargo build --releasepython3 scripts/version.py --check(all packages at1.15.0)python3 -m unittest discover -s scripts/tests(18 tests passed)cargo check --manifest-path wasm/Cargo.tomlgit diff --checkpdf-evalswas not run becausefirecrawl/pdf-evalsis not accessible to the current GitHub account (404). The installed Homebrew Rust toolchain also lacks thewasm32-unknown-unknowntarget and has norustup, so wasm32-target checks could not be rerun locally; the wasm package native check and both format checks pass. Maintainer-side pdf-evals and wasm32 CI would still be appreciated.AI assistance: OpenAI Codex helped inspect the aggregation path, implement the focused fix, and run validation. I reviewed the complete diff and take responsibility for the contribution.